Add minimal uiTable implementation for windows #361

bcampbell · 2018-05-05T11:04:08Z

(The first two commits are cherry-picked from the msys fixes - I needed them to build. The table changes are in a single commit)

This uses the win32 common controls listview to implement uiTable.
There are limitations:

It supports only a single TextPart per column.
ImagePart, CheckboxPart and ProgessBarPart are not implemented.
There is no support for cell coloring.
Cell editing is not implemented.
Some of these will be very hard to support using the standard
common control listview, and probably require an entire custom
listview.

msink · 2018-05-05T11:20:19Z

msys2 build is fixed on master branch - so maybe we need to merge andlabs:master into andlabs:table first.

andlabs · 2018-05-05T13:14:06Z

Yes, I'll merge those branches shortly.

andlabs · 2018-05-05T13:16:15Z

Done.

andlabs · 2018-05-05T13:19:34Z

windows/image.cpp

+struct uiImage {
+ double width;
+ double height;
+ // HIMAGELIST images;


Not a review comment, just a bit of information: I don't think we'll be able to use HIMAGELIST here, since that deals with a set of visually distinct images that have the same pixel sizes, while uiImage deals with a set of visually identical images that has different pixel sizes but the same physical size (for DPI independence). Don't worry about changing anything; I'll work on it later if you don't already have another PR with it.

Yep, I agree. Wasn't planning to go anywhere further with HIMAGELIST, Just stubbing out enough to get the tables compiling. and threw in the comment while I was in listview mode. I didn't spot the multi-size vs different-images distinction.

andlabs · 2018-05-05T13:21:00Z

windows/table.cpp

@@ -0,0 +1,277 @@
+#include "uipriv_windows.hpp"
+
+#include <vector>


This include is already in winapi.hpp, so it's not needed.

andlabs · 2018-05-05T13:21:59Z

Huh, does github still not let you see merge conflicts?

andlabs · 2018-05-07T01:34:47Z

I merged the Windows fixes into the table branch now; you may need to pull for that to be reflected.

bcampbell · 2018-05-07T08:56:49Z

I rebased my changes to try and keep the history clean. This is nudging against the outer limits of my git-fu, so fingers crossed :- )

andlabs · 2018-05-07T13:52:47Z

Seems to have worked. Will review.

andlabs

Mostly fine with the functionality this provides; most of these comments are consistency-related. Thanks again!

andlabs · 2018-05-07T13:56:11Z

windows/table.cpp

+ item.iItem = newIndex;
+ item.iSubItem = 0; //?
+ for (auto t : m->tables) {
+ ListView_InsertItem( t->hwnd, &item );


Consistency note: don't use the commctrl.h wrappers; use SendMessageW() explicitly.

if (SendMesageW(t->hwnd, LVM_INSERTITEM, 0, (LPARAM) (&item)) == (LRESULT) (-1)) logLastError(L"error calling LVM_INSERTITEM in uiTableModelRowInserted()");

(Note this also checks for errors; a thing I still need to rationalize properly.)

Likewise for consistency, no braces around single-line loop bodies

andlabs · 2018-05-07T14:01:05Z

windows/table.cpp

+void uiTableModelRowDeleted(uiTableModel *m, int oldIndex)
+{
+ for (auto t : m->tables) {
+ ListView_DeleteItem( t->hwnd, oldIndex );


Likewise here.

if (SendMesageW(t->hwnd, LVM_DELETEITEM, (WPARAM) oldIndex, 0) == (LRESULT) (FALSE)) logLastError(L"error calling LVM_DELETEITEM in uiTableModelRowDeleted()");

andlabs · 2018-05-07T14:01:37Z

windows/table.cpp

+void uiTableModelRowChanged(uiTableModel *m, int index)
+{
+ for (auto t : m->tables) {
+ ListView_Update( t->hwnd, index );


Likewise here.

if (SendMesageW(t->hwnd, LVM_UPDATE, (WPARAM) oldIndex, 0) == (LRESULT) (FALSE)) logLastError(L"error calling LVM_UPDATE in uiTableModelRowDeleted()");

andlabs · 2018-05-07T14:02:34Z

windows/table.cpp

+ }
+ }
+
+ LV_COLUMN lvc;


Consistency: declarations at the top

More important, though:

a) use LVCOLUMNW — LV_COLUMN is technically a deprecated name as of at least Vista if not earlier, and the W is consistency to explicitly use Unicode
b) you should call ZeroMemory(&lvc, sizeof (LVCOLUMNW)) before setting any fields (partially consistency, partially correctness)

andlabs · 2018-05-07T14:07:59Z

windows/table.cpp

+ lvc.fmt = LVCFMT_LEFT;
+ lvc.cx = 120; // TODO
+ lvc.pszText = c->name;
+ ListView_InsertColumn(c->t->hwnd, lvIndex, &lvc);


if (SendMessageW(c->t->hwnd, LVM_INSERTCOLUMN, lvIndex, (LPARAM) (&lvc)) == (LRESULT) (-1)) logLastError(L"error calling LVM_INSERTCOLUMN in uiTableColumnAppendTextPart()");

andlabs · 2018-05-07T14:33:11Z

windows/table.cpp

+ // Don't think that'll cut it when some cells have overlong data (eg
+ // stupidly long URLs). So for now, just hardcode a minimum:
+
+ x = 107; // in line with other controls


I would name these constants somehow, for consistency

andlabs · 2018-05-07T14:34:20Z

windows/table.cpp

+ }
+ break;
+ }
+ default:


No need for default: break;

andlabs · 2018-05-07T14:35:22Z

windows/table.cpp

+ switch (nm->code) {
+ case LVN_GETDISPINFO:
+ {
+ NMLVDISPINFO* di = (NMLVDISPINFO*)nm; 


Style consistency:

NMLVDISPINFO *di = (NMLVDISPINFO *) nm; LVITEM *item = &(di->item);

Note that the rest of this function isn't consistent either, but we can worry about that later.

andlabs · 2018-05-07T14:36:15Z

windows/table.cpp

+ uiTableModelColumnType typ = (*mh->ColumnType)(mh,t->model,mcol);
+ if (typ == uiTableModelColumnString) {
+ void* data = (*(mh->CellValue))(mh, t->model, row, mcol);
+ int n = MultiByteToWideChar(CP_UTF8, 0, (const char*)data, -1, item->pszText, item->cchTextMax);


This might be a problem. It should be using toUTF16(), but I forget what the behavior is if you overwrite the pszText pointer with your own... or when it's safe to free. Just mark this as a TODO; I'll probably resolve it a different way.

andlabs · 2018-05-07T14:39:10Z

windows/table.cpp

+ if (n>=item->cchTextMax) {
+ item->pszText[item->cchTextMax-1] = L'\0';
+ }
+ } else if (typ == uiTableModelColumnInt) {


Just a note: this won't be used for strings, but will be for progressbars. I will need to explicitly document that... Mark that as a TODO for now, anyway; this code can stay for now.

bcampbell · 2018-05-14T01:26:51Z

Just a quick note to assure you I'm still here ;-) It's been a crazy-busy week or so, so I've not been able to spend any time on this. The feedback is great and all makes sense to me, and I'm aiming to apply it all over the next few days.

andlabs · 2018-05-14T01:30:48Z

Okay; good luck with everything else going on!

This uses the win32 common controls listview to implement uiTable. There are limitations: - It supports only a single TextPart per column. - ImagePart, CheckboxPart and ProgessBarPart are not implemented. - There is no support for cell coloring. - Cell editing is not implemented. Some of these will be very hard to support using the standard common control listview, and probably require an entire custom listview.

andlabs

Mostly good to go; just a few minor stuff to fix up first. Thanks again!

I'll talk more about parts stuff on the main issue.

andlabs · 2018-05-27T01:41:30Z

test/page16.c

@@ -132,6 +132,8 @@ uiBox *makePage16(void)

 uiTableSetRowBackgroundColorModelColumn(t, 3);

+ uiTableAppendTextColumn(t, "Numbers", 8);


As I mentioned in another comment, the integer column type is for progress-bar parts. not for text strings. I don't remember what happens with other platforms though, so keep this around and I'll remove it if it causes issues.

However, this does raise the question of whether to provide helpers for displaying numbers as text. That might be outside the scope of libui, but it's still an interesting question...

It seems to me that the type of the cell used in the table display is independent from datatype of the column in the model. While most mappings between the data and display types are clearly pretty bonkers (eg colour => progress bar doesn't sound too useful), it doesn't seem unreasonable for the table to have a good stab at the sane ones (one?). Particularly the numeric => text mapping.
It's a pretty common case to want to display numeric data in a table, so forcing the client app to present it's numeric data as text when the model already has perfectly good support for numeric data seems like a recipe for confusion...

andlabs · 2018-05-27T01:41:52Z

windows/table.cpp

+
+//static void uiTableMinimumSize(uiWindowsControl *c, int *width, int *height);
+
+struct uiTable;


No need for this; ui_table.h already does this.

andlabs · 2018-05-27T01:42:16Z

windows/table.cpp

@@ -0,0 +1,295 @@
+#include "uipriv_windows.hpp"
+
+//static void uiTableMinimumSize(uiWindowsControl *c, int *width, int *height);


No need for this; ui_windows.h already does this.

andlabs · 2018-05-27T01:45:04Z

windows/table.cpp

+ ZeroMemory(&item, sizeof (LVITEMW));
+ item.mask = 0;
+ item.iItem = newIndex;
+ item.iSubItem = 0; //?


0 appears to be correct here. yes, since a subitem index of 0 means the item itself, not any of its subitems. (The leftmost column is not considered as "subitem" because of how listview is basically an extended listbox.)

andlabs · 2018-05-27T01:45:21Z

windows/table.cpp

+ item.mask = 0;
+ item.iItem = newIndex;
+ item.iSubItem = 0; //?
+ for (auto t : m->tables) {


Style consistency nit: no need for braces here since the body is one statement.

andlabs · 2018-05-27T01:46:05Z

windows/table.cpp

+
+void uiTableModelRowDeleted(uiTableModel *m, int oldIndex)
+{
+ for (auto t : m->tables) {


Style consistency nit: no need for braces here since the body is one statement.

andlabs · 2018-05-27T01:46:26Z

windows/table.cpp

+ int lvIndex = 0;
+ LVCOLUMNW lvc;
+
+ if (c->modelColumn >=0) {


Style consistency nits: space after >=, no need for braces here since the body is one statement. I'll stop this second one for the rest of the review, but it still applies.

andlabs · 2018-05-27T01:49:40Z

windows/table.cpp

+ *width = x;
+ *height = y;
+}
+


Style consistency nit applied to everything: use only a single blank line between functions. not two.

andlabs · 2018-05-27T01:59:18Z

windows/table.cpp

+// Don't think that'll cut it when some cells have overlong data (eg
+// stupidly long URLs). So for now, just hardcode a minimum:
+#define tableMinWidth 107 /* in line with other controls */
+#define tableMinHeight (14*3) /* header + 2 lines (roughly) */


There might be a way to use LVM_GETHEADER to get the necessary size of the header control, perhaps combined with HDM_LAYOUT, but I'm not sure how this cooperates with listview controls. Mark this as a TODO for now.

andlabs · 2018-05-27T02:00:57Z

windows/table.cpp

+ SendMessageW(t->hwnd, LVM_SETEXTENDEDLISTVIEWSTYLE,
+ (WPARAM) (LVS_EX_FULLROWSELECT | LVS_EX_LABELTIP),
+ (LPARAM) (LVS_EX_FULLROWSELECT | LVS_EX_LABELTIP));
+ // TODO: try LVS_EX_AUTOSIZECOLUMNS


Move this above the SendMessage() call.

bcampbell · 2018-05-28T08:42:31Z

OK, I think that addresses them all. Let me know if I missed any!

andlabs

One last comment.

andlabs · 2018-05-28T11:59:13Z

windows/table.cpp

@@ -130,7 +119,7 @@ void uiTableColumnAppendProgressBarPart(uiTableColumn *c, int modelColumn, int e

 void uiTableColumnPartSetEditable(uiTableColumn *c, int part, int editable)
 {
- // TODO


Keep this as a TODO for now, so that it shows up in my occasional grep TODOs and thus isn't forgotten.

andlabs · 2018-05-29T23:58:53Z

All right; thanks! I'll merge this into table now at last, though there are a few other minor changes and one last runthrough before I merge this into master. Thanks a lot again!

bcampbell mentioned this pull request May 5, 2018

Master issue for Table discussion #310

Open

5 tasks

andlabs reviewed May 5, 2018

View reviewed changes

andlabs mentioned this pull request May 7, 2018

Table #343

Closed

bcampbell force-pushed the table-comctl branch from 26abba2 to 70c1de3 Compare May 7, 2018 08:54

andlabs requested changes May 7, 2018

View reviewed changes

bcampbell added 2 commits May 22, 2018 20:41

remove superfluous #include <vector>

7402dec

bcampbell force-pushed the table-comctl branch from 70c1de3 to 7402dec Compare May 22, 2018 09:37

assorted consistancy cleanup

4246ae5

andlabs requested changes May 27, 2018

View reviewed changes

further style consistency tweaks

ba13227

andlabs reviewed May 28, 2018

View reviewed changes

andlabs mentioned this pull request May 28, 2018

Create a uiImage control #209

Open

minor windows table tweaks

b3b2119

andlabs merged commit 32a8324 into andlabs:table May 30, 2018

cody271 mentioned this pull request Jan 18, 2022

Use Win32 macro wrappers instead of SendMessage() libui-ng/libui-ng#35

Open

		@@ -0,0 +1,277 @@
		#include "uipriv_windows.hpp"

		#include <vector>

		@@ -132,6 +132,8 @@ uiBox *makePage16(void)

		uiTableSetRowBackgroundColorModelColumn(t, 3);

		uiTableAppendTextColumn(t, "Numbers", 8);


		//static void uiTableMinimumSize(uiWindowsControl c, int width, int *height);

		struct uiTable;

		@@ -0,0 +1,295 @@
		#include "uipriv_windows.hpp"

		//static void uiTableMinimumSize(uiWindowsControl c, int width, int *height);

Add minimal uiTable implementation for windows #361

Add minimal uiTable implementation for windows #361

Conversation

bcampbell commented May 5, 2018

msink commented May 5, 2018

andlabs commented May 5, 2018

andlabs commented May 5, 2018

andlabs May 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andlabs commented May 5, 2018 • edited Loading

andlabs commented May 7, 2018 • edited Loading

bcampbell commented May 7, 2018

andlabs commented May 7, 2018

andlabs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcampbell commented May 14, 2018

andlabs commented May 14, 2018

andlabs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcampbell commented May 28, 2018

andlabs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andlabs commented May 29, 2018

andlabs May 5, 2018 •

edited

Loading

andlabs commented May 5, 2018 •

edited

Loading

andlabs commented May 7, 2018 •

edited

Loading